Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Larkin: Layout independent bindings #56

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

haberdashPI
Copy link
Owner

@haberdashPI haberdashPI commented Jan 13, 2025

Closes #54, addressing the following points:

  1. Replace all uses of keys in Larkin with the layout independent versions
  2. Nice to have: make sure it is easy to ignore all these layout independent versions via foreach. I think this might already be possible by using foreach.key = ['{key: [[A-Za-z0-9]+]}', 'shift+{key: [[A-Za-z0-9]+]}'], but I need to verify that it is working (and fix it if it isn't).
  3. Handle display of layout independent bindings in the Masterkey Visual Documentation: Keep US layout but verify all is well
  4. Verify that the layout independent bindings display properly in the quick pick view — fix them if they aren't working

Remaining TODOs:

  • Cleanup partial layout independence (e.g. replace [r] with [KeyR]).
  • Review layout independence of all keys in Larkin.toml
  • Create layout dependent version of Larking.toml
  • Think through discoverability of layout independence. Right now I'm not super convinced it will be clear to users.
  • Fix false positives for (U.S. Layout) suffix
  • Verify that the layout features are working in the wild (not just in tests).

@haberdashPI haberdashPI marked this pull request as draft January 13, 2025 02:25
@haberdashPI haberdashPI force-pushed the push-sszrsmrxzzwo branch 8 times, most recently from 608db6f to 569e096 Compare January 18, 2025 17:12
@haberdashPI haberdashPI marked this pull request as ready for review January 18, 2025 17:13
@haberdashPI
Copy link
Owner Author

haberdashPI commented Jan 20, 2025

Note for WIP

Debugging using version 1.92 of VSCode, as that is the last version that supports debugging a web extension right now (reported bug is merged, but needs to make into a release—it's not even in the insiders release yet it seems).

foreach of digits is not yet working, guessing this is some issue with escaping the brackets in the regex.

@haberdashPI haberdashPI force-pushed the push-sszrsmrxzzwo branch 2 times, most recently from bcd9f34 to f4840d2 Compare January 22, 2025 04:19
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.83%. Comparing base (8e0b701) to head (ee5d849).

Files with missing lines Patch % Lines
src/web/commands/palette.ts 85.71% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #56      +/-   ##
==========================================
+ Coverage   79.44%   79.83%   +0.39%     
==========================================
  Files          24       25       +1     
  Lines        2340     2371      +31     
  Branches      468      473       +5     
==========================================
+ Hits         1859     1893      +34     
+ Misses        292      289       -3     
  Partials      189      189              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@haberdashPI haberdashPI force-pushed the push-sszrsmrxzzwo branch 4 times, most recently from 57183e1 to 4850dc3 Compare January 25, 2025 21:05
@haberdashPI
Copy link
Owner Author

@sanarise: This branch is now in working order, and includes support for all of the bullets listed above. Would you be willing to take this version out for a "test drive"??

  • see if your own keybinding file works well with these change
  • try out the default (newly layout independent) Larking bindings to see if you find any issues working with it for just a little bit.

Happy to help walk you through the steps to get this to compile and install for you.

@sanarise
Copy link

Hi! Yes, apparently I need instructions on how to compile this branch from source and connect it to my vscode. I've cloned the repository and played around with the branch and various commands from the package.json, but I haven't figured out yet how to get the compiled module and how to connect it to my vscode. Seems like its need to build a vsix file/module?
2025-01-27_10-00-37

@haberdashPI
Copy link
Owner Author

First step should be to follow the directions at the end of the README.

Once those steps are complete you can run

mise use vsce
vsce package

And you should have a VSIX package you can install.

Alternatively, you could also run the "Debug: Start Debugging" command within the open VSCode project.

@sanarise
Copy link

  1. I have an error when trying to use vsce in mise
    2025-01-28_12-09-00
  2. I started the project in debug mode, and tried my preset - everything seems to work with LI bindings. But if you replace "[Semicolon]" with ";" in the rule, it works in the English layout, but not in the Russian one. At the same time, in the modline, when the key is pressed, it shows "[;]" and, accordingly, this is not worked out in any way. Is this a bug or a feature?

@sanarise
Copy link

I'm not sure that the idea I'm going to propose now is clear in itself and has no pitfalls. But now there is still one oddity in the behavior.
For letter keys, you can bind both "a" and "KeyA" and everything will work in all layouts. It's been like this since the beginning. And it masks the presence of this problem, which is also bad. For non-letter ones, only "[Semicolon]" works smoothly, but ";" also causes a problem in national layouts.

Maybe you should think about hiding this problem from the user altogether? That is, at the toml preset level, everything remains as it is (without LI-bindings), and inside the MasterKey, always translate to LI-bindings. Can there be any arguments against this? That is, we may not be able to map to LI-bindings under some conditions, but it's still hard for me to imagine when it's needed...

@haberdashPI
Copy link
Owner Author

haberdashPI commented Jan 28, 2025

have an error when trying to use vsce in mise

Ah, I forgot. It is mise use npm:vsce. Whoopsie.

Maybe you should think about hiding this problem from the user altogether?

I'm not convinced. It is a little unintuitive that you might have a binding for one letter or symbol, but you hit the key for some other symbol on your keyboard to use that binding: it might be something developers with international layouts want but a user new to all of this could find it very confusing (and maybe think it's a bug).

For non-letter ones, only "[Semicolon]" works smoothly, but ";" also causes a problem in national layouts.

What do you mean only [Semicolon] works smoothly?

it shows "[;]" and, accordingly, this is not worked out in any way

I'm a little lost about what you mean here. The intent is that [;] means you are pressing what would be, in a US Layout, the ; key. There isn't any easy way to know what key it is for any given layout (I don't think extensions have access to the current layout).

What is the behavior you were expecting?

Maybe you could spell out the following for a few key examples:

  • list what key you are hitting on your keyboard
  • what key you expect it to appear to the extension while in normal mode
  • what key the extension actually registers

@sanarise
Copy link

sanarise commented Jan 30, 2025

I built the module in vsix and tested my preset – it seems to be working well. I didn't find any bugs. The key lights in Visual Documentation have also started working.

Now for the essence of what I wrote about. There's only one thing that bothers me about this whole story at the moment. And this one looks just like some kind of bug, although it doesn't bother much. Below I will give a piece of my preset. You may notice that right now I'm using w instead of [KeyW], shift+w instead of shift+[KeyW] – and it works. And it worked from the beginning. Therefore, the problem was not immediately noticed.
In this case, [Semicolon], [Period], shift+[Period] I have to use it because otherwise these buttons won't work in the Ru layout.

I do not know why this is so.... This feature is present initially. There was no problem with letter keys initially, but I noticed a problem with non-letter keys.

[[bind]]
path = "visualMode.motion"
key = "[Semicolon]"
name = "→eol"
command = "cursorEndSelect"

[[bind]]
path = "visualMode.motion.obj"
key = "w"
name = "→|word"
args.unit = "word"
computedArgs.value = "count || 1"

[[bind]]
path = "visualMode.motion.obj"
key = "shift+w"
name = "→|subw"
args.unit = "subword"
computedArgs.value = "count || 1"

[[bind]]
path = "visualMode.motion.obj"
key = "b"
name = "←|word"
args.unit = "word"
computedArgs.value = "-count || -1"

[[bind]]
path = "visualMode.motion.obj"
key = "shift+b"
name = "←|subw"
args.unit = "subword"
computedArgs.value = "-count || -1"

[[bind]]
path = "visualMode.motion.obj"
key = "e"
name = "→word|"
args.unit = "word"
args.boundary = "end"
computedArgs.value = "count || 1"

[[bind]]
path = "visualMode.motion.obj"
key = "shift+e"
name = "→subw|"
args.unit = "subword"
args.boundary = "end"
computedArgs.value = "count || 1"

[[bind]]
path = "visualMode.motion.obj"
key = "[Period]"
name = "↓parag"
description = "next paragraph"
args.unit = "paragraph"
computedArgs.value = "count || 1"

[[bind]]
path = "visualMode.selection.obj"
key = "shift+[Period]"
name = "↓parag"
description = "next paragraph"
args.commands = [
    "selection-utilities.activeAtEnd",
    { command = "selection-utilities.moveBy", args = { unit = "paragraph", select = true, value = "1" } },
]

[[bind]]
path = "visualMode.motion.obj"
key = "[Comma]"
name = "↑parag"
description = "previous paragraph"
args.unit = "paragraph"
computedArgs.value = "-(count || 1)"

[[bind]]
path = "visualMode.selection.obj"
key = "shift+[Comma]"
name = "↑parag"
description = "previous paragraph"
args.commands = [
    "selection-utilities.activeAtStart",
    { command = "selection-utilities.moveBy", args = { unit = "paragraph", select = true, value = "-1" } },
]

@sanarise
Copy link

If this moment doesn't bother you, then fine – maybe let it stay that way. But it looks as if the letter keys are originally mapped to LI-mnemonics somewhere)) Otherwise, why don't they reproduce this problem initially?

Of course, I don't know if they are actually maps or not. And the exact reason for this behavior is unknown to me.

@sanarise
Copy link

sanarise commented Jan 30, 2025

That's why I suggested thinking about what could map everything to LI mnemonics by default, so that it would be somehow coherent and unambiguous. But here, of course, it would be good to first understand the true reason for the behavior of the letter keys.

Again, it's up to you to decide how important it is to deal with this right now. Perhaps this is a rather minor priority, because this misunderstanding does not particularly interfere with work.

@haberdashPI
Copy link
Owner Author

Oh! Okay, I think I understand. You're saying that the alphanumeric keys specified normally (without []) use a layout independent binding (so pressing the key that would be w in a U.S. layout causes the command mapped to w to trigger). Is that right?? Intersting... was that true before this PR? If you install extension from the store it also works this way??

I wonder if a possible solution here is that there could be a flag, either specified in the keymap or as a preference for this extension, which maps all bindings to their layout independent format. Users can toggle this on or off as they choose.

@sanarise
Copy link

sanarise commented Feb 1, 2025

Oh! Okay, I think I understand. You're saying that the alphanumeric keys specified normally (without []) use a layout independent binding (so pressing the key that would be w in a U.S. layout causes the command mapped to w to trigger). Is that right??

Yes, absolutely right. I've written about this before too. But apparently due to the difficulties of translation, this point escaped our attention )

Intersting... was that true before this PR? If you install extension from the store it also works this way??

Yes, this has been the case since the very beginning, when I started using the Master Key. I've now checked on version 0.3.7 – that's exactly how it works. And at that moment, I was pleased with this behavior, because I no longer needed an additional fix external to vscode, which I used with aka.ms/vscodevim I noticed the problem with non-letter keys later.
By the way, nothing works in vscodevim in the Russian layout. That is, neither alphabetic nor other keys work. Therefore, for vscodevim to work, an additional module was used that switches the layout to US when exiting input mode. I mean, this probably hints that the reason for the operation of letter keys in early versions of MasterKey is localised somewhere inside MasterKey )

I wonder if a possible solution here is that there could be a flag, either specified in the keymap or as a preference for this extension, which maps all bindings to their layout independent format. Users can toggle this on or off as they choose.

Yes, I've been thinking about that too. Perhaps it would be really good to give such a choice to the user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The problem with self-input non-alphabetic keys in non-latin keyboard layouts
2 participants